Hi @kornerc,

Thank you for the contribution. We actually have been thinking about doing this in the past. For some configuration files (e.g. code_generator/*, CMakePresets.json and tests/**/params.json), this is a good improvement.

However, for the data files (input.json, update.json, sym_output.json, asym_output.json and sc_output.json), this is not necessarily an improvement. The sheer amount of added data as a direct consequence of nesting is quite large. E.g.:

• the amount of added lines (about 170k; obtained from the summary +215,469 −44,959); every line contains a number of whitespaces due to nesting.
• your finding that 1e15 is expanded to 1000000000000000.0

This exact reason is why we deliberately use a compact notation with a maximum indentation level (see this formatter and its configuration).

That said, having consistent formatting helps with both code quality and code reviews. Here are a couple potential paths going forward:

• make it so that the formatter also uses a max indentation; or
• if you can use the PGM (de)serializer to reformat (e.g. by deserializing and then serializing again); or
• if neither is possible, then maybe we can restrict the formatting to only include files that are not PGM datasets.

One last remark: to keep the git commit history clean, can you please either squash the commits after you're done reverting (at the latest when sending it to the merge queue) or open a new PR?

Hi @kornerc, thanks for proposing this new improvement.

In addition of what @mgovers said, I would say ignore all PGM data json files. These files already have a custom indent pattern which is produced by the serializer. Also, user/contributor might want to a specific custom indent for a certain data file for some reason (usually modelling or readability). Enforcing a rule on PGM data json files does not make sense.

So I would go for option 3 of @mgovers proposed:

• we can restrict the formatting to only include files that are not PGM datasets.

Hi @mgovers and @TonyXiang8787 thank you for your review!

Based on your feedback I made some changes:

• I reverted the last commits and force pushed and followed as suggested option 3

  we can restrict the formatting to only include files that are not PGM datasets.

• Initially I used the pre-commit hook pretty-format-json.
  Even though it is quite simple, its configuration is rather limited and I was not too happy that it made major changes on numbers e.g. 1e15 -> 1000000000000000.0
• Thus, I searched for an alternative and found Biome which convinced me:
  • Empty lines are kept
  • Numbers are only slightly changed e.g. 1e-08 -> 1e-8
  • A pre-commit hook is available
  • An extension for VS Code exists
  • Standalone executeables are available
  • It's possible to overwrite the configuration for certain files e.g. allow comments in .json files located in .vscode
• I again split the changes into 2 commit:
  • 7652dce with the relevant changes
  • c0e5e33 where the formatter is applied on the repo
• Instead of whitelistening the files to format, I blacklisted the PGM dataset files
• I added a step in the check-code-quality action to run the formatter
• I didn't manage to set up the VS Code extension in the devcontainer properly and instead adjusted the default formatter in .vscode/settings.json to function similar to Biome. I can configure the devcontainer for Biome properly in another PR.
• I've noticed in the last run of the actions that Biome formats irrelevant files e.g. files in the .mypy_cache.
  I can add these folders to the blacklisted files after the rest of the PR is approved so that I can still easily revert the commit which formats all the files.

I am happy to hear your thoughts!

Hi @kornerc,

We would prefer a formatter which can be installed from PyPI. In that way, our automatic dependency refresh can make sure that the dev environment, pre-commit, and CI all use the same version of the formatter.

Your proposal of biome does not provide a PyPI package. So we need to sync the formatter versions manually. Do you see other possibilities from PyPI? Or do you see a way to also automate the formatter/linter versions from nodejs?

Hi @kornerc,

After some digging. I think we can add npm structure into the repo to keep track of versioning.

So first create a package.json

{
  "private": true,
  "devDependencies": {
    "biomejs/biome": "*",
    "markdownlint-cli": "*"
  }
}

Add node_modules/ to .gitignore.

Then run npm install locally once so we can also get package-lock.json to be check in to VCS.

Then we can change CI code quality, to install npm packages locally without -g and change the markdownlint and biome call with prefix of npm run.

Then we modify the refresh dependency job to also refresh npm packages including adjusting pre-commit.yaml.

Finally change the build guide to instruct developers use npm to install the formatter.

I didn't manage to set up the VS Code extension in the devcontainer properly and instead adjusted the default formatter in .vscode/settings.json to function similar to Biome. I can configure the devcontainer for Biome properly in another PR.

i think that that is alright, but with the existing configuration, i do get suggestions along the lines of

 {
-  "calculation_method": ["linear", "newton_raphson", "iterative_current", "linear_current"],
+  "calculation_method": [ "linear", "newton_raphson", "iterative_current", "linear_current" ],
   "rtol": 1e-5,
   "atol": 1e-5
 }

If you want to postpone recommendation for Biome, then please try to see whether you can turn off those extra whitespace suggestions, or otherwise whether you can configure either Biome to add them. If neither of the options are feasible, it's fine to keep it as-is for now.

Unfortunately this is giving me some headaches:

• VS Code Biome Extension: Somehow I am having problems formatting .json files with the VS Code extension
  🐛 JSON and JSONC formatting with VS Code extension: "Extension 'Biome' is configured as formatter but it cannot format 'JSON with Comments'-files" biomejs/biome-vscode#908
• Biome missing configuration option: currently Biome has no option to add spaces into arrays
  📎 Implement formatting option to include space inside parens, braces, brackets, etc. biomejs/biome#4607
• VS Code missing option: currently the .json formatter of VS Code does not have an option to disable the spaces in arrays
  [json] formatter: allow to specify whether spaces are added for arrays with single item microsoft/vscode#247441

I would suggest to 1) get this PR merged where the VS Code formatter is used in the devcontainer and 2) write an issue so that I can tackle this as soon the issue with the Biome extension is clarified.

Update: I managed to setup the VS-Code extension (Biome needs to be installed).
I will integrate it in the devcontainer.

I've noticed in the last run of the actions that Biome formats irrelevant files e.g. files in the .mypy_cache.
I can add these folders to the blacklisted files after the rest of the PR is approved so that I can still easily revert the commit which formats all the files.

good choice. Please indeed blacklist them.

Biome now parses the .gitignore file and ignores the files and folders specified in it.

After some digging. I think we can add npm structure into the repo to keep track of versioning.

Yes I like the idea to also automatically update the node dependencies.
Would it be okay if I try to implement this for Biome and markdownlint in a different PR, since I don't think that I find the time in the next week to address this?

Finally change the build guide to instruct developers use npm to install the formatter.

This should not be needed.
pre-commit takes care about setting everything up for the Biome formatter hook.
The VS Code Biome extension ships with everything which is needed to run Biome (I guess it is the Biome binary (programmed in Rust)).
Thus, the formatting should work with pre-commit and with the VS Code extension without installing node.

Update: The VS Code extension needs Biome to be installed I will have a look if it is able to use the Biome binary installed with precommit.

Hi @kornerc, Thank you again for the extensive investigation and update. Let us know if there's anything you need.